⚠️ No Changeset found

Latest commit: 7854d97

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

• perf(es/compat): Merge optional_catch_binding #11313 👈 (View in Graphite)
• perf(es/compat): Merge object_rest_spread #11312
• perf(es/compat): Merge async_to_generator #11311
• perf(es/compat): Merge exponentation_operator #11310
• perf(es/compat): Merge export_namespace_from to Transformer #11309
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Code Review: perf(es/compat): Merge optional catch binding

Summary

This PR attempts to merge the optional catch binding transformation into the new transformer architecture. However, there is a critical bug: the transformation logic has been completely removed, making this a breaking change that will cause the feature to stop working.

🚨 Critical Issues

1. Missing Implementation (BLOCKING)

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs

The new implementation is completely empty:

pub fn hook() -> impl VisitMutHook<TraverseCtx> {
    OptionalCatchBindingPass {}
}

struct OptionalCatchBindingPass {}

impl VisitMutHook<TraverseCtx> for OptionalCatchBindingPass {}

The old implementation (now removed from swc_ecma_compat_es2019/src/optional_catch_binding.rs) actually performed the transformation:

fn visit_mut_catch_clause(&mut self, cc: &mut CatchClause) {
    cc.visit_mut_children_with(self);

    if cc.param.is_some() {
        return;
    }
    cc.param = Some(private_ident!("e").into());
}

Impact: Optional catch binding transformation will not work at all. Code like try {} catch {} will not be transpiled to try {} catch(e) {}.

Fix Required: The new hook implementation must override the enter_catch_clause or exit_catch_clause method from VisitMutHook<TraverseCtx>:

impl VisitMutHook<TraverseCtx> for OptionalCatchBindingPass {
    fn enter_catch_clause(&mut self, node: &mut CatchClause, _ctx: &mut TraverseCtx) {
        if node.param.is_none() {
            node.param = Some(private_ident!("e").into());
        }
    }
}

2. Test Failures (BLOCKING)

Location: crates/swc_ecma_compat_es2019/src/optional_catch_binding.rs:20-26

The test code still references the old implementation:

use crate::optional_catch_binding::OptionalCatchBinding;

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        visit_mut_pass(OptionalCatchBinding),
    )
}

Issue: OptionalCatchBinding struct no longer exists, so these tests won't compile.

Fix Required: Tests should use the new public API (optional_catch_binding() function), or they should be moved to the transformer crate.

3. Missing Hook Registration in OptionalHook (POTENTIAL ISSUE)

Location: crates/swc_ecma_transformer/src/hook_utils.rs:30-44

The OptionalHook wrapper only implements hooks for:

• Expr
• Pat
• Stmt
• ModuleItem
• Module
• Script
• Program

It does not implement enter_catch_clause/exit_catch_clause. The comment at line 30 acknowledges this:

// TODO: Implement lots of hooks, or move it to `swc_ecma_hooks`

Impact: Even if you implement enter_catch_clause in OptionalCatchBindingPass, it won't be called because OptionalHook doesn't forward catch clause visits.

Fix Required: Add CatchClause hooks to OptionalHook:

optional_method!(enter_catch_clause, exit_catch_clause, CatchClause);

Other Issues

4. Inconsistent Dependency Direction

Location: crates/swc_ecma_compat_es2019/Cargo.toml:21

Adding swc_ecma_transformer as a dependency of swc_ecma_compat_es2019 creates a somewhat circular architecture:

• swc_ecma_preset_env depends on swc_ecma_compat_es2019
• swc_ecma_compat_es2019 now depends on swc_ecma_transformer
• But swc_ecma_transformer already has the ES2019 implementation

Recommendation: Consider fully migrating this transform to swc_ecma_transformer and having swc_ecma_compat_es2019 just re-export the function, or remove it entirely if backwards compatibility isn't needed.

5. Missing Documentation

Per CLAUDE.md guideline #7: "Write documentation for your code."

The new hook implementation has no documentation explaining:

• What transformation it performs
• Why the merge improves performance
• How it integrates with the new transformer architecture

Performance Considerations

The PR title mentions "perf" but there's no evidence of performance improvement:

• No benchmarks provided
• The new implementation doesn't exist, so there's no performance to measure
• The integration with the hook system may have overhead

Recommendation: After fixing the implementation, provide benchmark results showing the performance improvement from merging this pass.

Test Coverage

❌ Tests are broken - they reference deleted code (OptionalCatchBinding struct)

Required:

1. Fix or update existing tests
2. Verify all test cases still pass:
   • issue_411: try {} catch {}
   • catch_binding_name_collision_1: collision with variable e
   • catch_binding_name_collision_2: collision with outer var e

Security Concerns

✅ No security issues identified - this is a straightforward AST transformation.

Recommendations

1. Implement the actual transformation logic in OptionalCatchBindingPass::enter_catch_clause
2. Add CatchClause hooks to OptionalHook in hook_utils.rs
3. Fix or migrate the tests in swc_ecma_compat_es2019
4. Run cargo test --package swc_ecma_compat_es2019 to verify tests pass
5. Run cargo fmt --all before committing (per CLAUDE.md)
6. Add documentation explaining the transformation and performance benefits
7. Consider architecture: Should swc_ecma_compat_es2019 depend on the transformer, or should it be fully migrated?

Verdict

❌ CHANGES REQUESTED - This PR cannot be merged in its current state due to the missing implementation and broken tests.

The refactoring approach is good, but the execution is incomplete. Once the transformation logic is implemented and tests are fixed, this should be ready for merge.

CodSpeed Performance Report

Merging #11313 will not alter performance

_{Comparing kdy1/optional-catch-binding (d29b1c5) with kdy1/object-rest-spread (3faf520)}

Summary

✅ 138 untouched

Code Review: PR #11313 - perf(es/compat): Merge optional_catch_binding

Critical Issues 🚨

1. Missing Implementation - The transformation does nothing

The new OptionalCatchBindingPass in crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs is an empty struct with no implementation:

struct OptionalCatchBindingPass {}

impl VisitMutHook<TraverseCtx> for OptionalCatchBindingPass {}

The original implementation had actual logic to add a parameter to catch clauses:

fn visit_mut_catch_clause(&mut self, cc: &mut CatchClause) {
    cc.visit_mut_children_with(self);
    if cc.param.is_some() {
        return;
    }
    cc.param = Some(private_ident!("e").into());
}

This needs to be ported to the hook-based system.

2. OptionalHook Missing CatchClause Support

The OptionalHook wrapper in crates/swc_ecma_transformer/src/hook_utils.rs only implements hooks for:

• Expr
• Pat
• Stmt
• ModuleItem
• Module
• Script
• Program

But it's missing enter_catch_clause/exit_catch_clause hooks! This means even if the implementation existed, it wouldn't be called. You need to add:

optional_method!(enter_catch_clause, exit_catch_clause, CatchClause);

to the OptionalHook implementation around line 44.

3. Broken Tests

The tests in crates/swc_ecma_compat_es2019/src/optional_catch_binding.rs:20-26 reference OptionalCatchBinding which no longer exists:

use crate::optional_catch_binding::OptionalCatchBinding;

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        visit_mut_pass(OptionalCatchBinding),
    )
}

This will fail to compile. The tests should use the new optional_catch_binding() function instead.

Code Quality Issues

4. Circular Dependency

Adding swc_ecma_transformer as a dependency of swc_ecma_compat_es2019 creates an odd circular reference since swc_ecma_transformer likely depends on compat modules. This suggests the architectural approach might need reconsideration.

5. Missing Documentation

The new implementation has no comments explaining:

• Why the merge is being done
• How the hook-based system works
• What the expected behavior is

Per CLAUDE.md guideline #7: "Write documentation for your code."

Performance Considerations

The PR title mentions this is a performance improvement ("perf"), but:

• No benchmarks are provided
• The approach seems incomplete, so performance can't be validated
• The hook-based system should theoretically be faster, but that needs verification

Recommendations

1. Implement the actual transformation logic in OptionalCatchBindingPass:

   impl VisitMutHook<TraverseCtx> for OptionalCatchBindingPass {
       fn enter_catch_clause(&mut self, node: &mut CatchClause, _ctx: &mut TraverseCtx) {
           if node.param.is_none() {
               node.param = Some(private_ident!("e").into());
           }
       }
   }

2. Add CatchClause support to OptionalHook in hook_utils.rs

3. Fix the tests to use the correct API

4. Add unit tests to verify the hook works correctly (per CLAUDE.md Handle early errors in ecmascript parser #5)

5. Run the existing tests to ensure no regressions (per CLAUDE.md Internal documentations #6)

6. Run cargo fmt --all before committing (per CLAUDE.md Unignore "431ecef8c85d4d24.js" and "8386fbff927a9e0e.js". #8)

7. Consider the architectural implications of the dependency structure

Security Concerns

None identified - this is a straightforward syntax transformation.

Test Coverage

The existing tests are good, but they need to be fixed to work with the new implementation. Once fixed, they should provide adequate coverage for this feature.

Verdict: This PR is incomplete and will not work in its current state. The transformation logic is missing, and the supporting infrastructure needs updates. Please address the critical issues above before merging.

Pull Request Review

Thank you for working on this performance optimization! The goal of merging optional_catch_binding into the main transformer architecture is good, but there are several critical issues that need to be addressed before this PR can be merged.

🚨 Critical Issues

1. Missing Implementation Logic

The new implementation in crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs is completely empty - it only defines an empty struct with no transformation logic.

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:9-11

struct OptionalCatchBindingPass {}

impl VisitMutHook<TraverseCtx> for OptionalCatchBindingPass {}

Problem: The actual transformation logic that adds a parameter to catch clauses without bindings has been removed from the old implementation but not added to the new one.

Expected: The hook needs to implement the enter_catch_clause or exit_catch_clause method to transform catch clauses:

impl VisitMutHook<TraverseCtx> for OptionalCatchBindingPass {
    fn enter_catch_clause(&mut self, cc: &mut CatchClause, _ctx: &mut TraverseCtx) {
        if cc.param.is_none() {
            cc.param = Some(private_ident!("e").into());
        }
    }
}

2. OptionalHook Doesn't Support CatchClause

The OptionalHook wrapper used in es2019/mod.rs:14 only implements hooks for a limited set of AST nodes. Looking at hook_utils.rs:32-44, it only supports: Expr, Pat, Stmt, ModuleItem, Module, Script, and Program.

Location: crates/swc_ecma_transformer/src/hook_utils.rs:26-45

Problem: CatchClause is not in the supported list, so even if the implementation were correct, the hook would never be called.

Solution: Either:

• Add support for CatchClause to OptionalHook:

  optional_method!(enter_catch_clause, exit_catch_clause, CatchClause);

• Or use a different hook composition pattern that supports all AST nodes

3. Broken Test References

The test file still references the old OptionalCatchBinding struct that was removed.

Location: crates/swc_ecma_compat_es2019/src/optional_catch_binding.rs:20,25

use crate::optional_catch_binding::OptionalCatchBinding;
// ...
visit_mut_pass(OptionalCatchBinding),

Problem: This will cause compilation errors because OptionalCatchBinding no longer exists.

📋 Code Quality Issues

4. Missing Documentation

Per CLAUDE.md rule #7, the new code should have documentation explaining what it does.

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs

Recommendation: Add module-level documentation:

//! ES2019: Optional catch binding
//!
//! Transforms catch clauses without bindings into catch clauses with a parameter.
//! This allows `try {} catch {}` to work in environments that don't support ES2019.

5. Missing Unit Tests

Per CLAUDE.md rule #5, new code should have unit tests. The transformation logic should be tested in the new location.

🔍 Testing Concerns

Critical: The existing tests will fail because:

1. The transformation logic is missing
2. The test helper references a deleted struct
3. The hook won't be invoked due to OptionalHook limitations

Recommendation: Run the tests before submitting:

cargo test --package swc_ecma_compat_es2019 optional_catch_binding

✅ What's Good

• The overall architecture change is sound - consolidating transformations is good for performance
• The option-based enabling/disabling mechanism (Es2019Options::optional_catch_binding) is clean
• The integration into preset_env looks correct

🛠️ Action Items

1. Implement the transformation logic in OptionalCatchBindingPass
2. Add CatchClause support to OptionalHook or use a different hook composition
3. Fix the test file to remove references to the deleted OptionalCatchBinding struct
4. Add documentation per project guidelines
5. Run tests to verify the transformation works correctly
6. Run cargo fmt --all before committing (per CLAUDE.md rule Unignore "431ecef8c85d4d24.js" and "8386fbff927a9e0e.js". #8)

Would you like help implementing these fixes?

Binary Sizes

Commit: 946461d

Code Review: PR #11313 - perf(es/compat): Merge optional_catch_binding

Summary

This PR migrates the optional_catch_binding transform from the legacy visitor pattern to the new hook-based transformer architecture. The goal is a performance improvement by consolidating transforms. However, there are critical issues that prevent this PR from compiling and functioning correctly.

🚨 Critical Issues (Blocking)

1. Type Error - Compilation Failure ❌

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:18

node.param = Some(Pat::Ident(private_ident!("unused")));
                          ^^^^^^^^^^^^^^^^^^^^^^^^ 
// Error: expected BindingIdent, found Ident

Issue: Pat::Ident expects a BindingIdent, but private_ident! returns an Ident.

Fix: Add .into() to convert Ident to BindingIdent:

node.param = Some(private_ident!("unused").into());

Reference: See similar usage in crates/swc_ecma_compat_es2015/src/destructuring.rs:314 and crates/swc_ecma_minifier/src/compress/optimize/arguments.rs:151.

2. Broken Tests ❌

Location: crates/swc_ecma_compat_es2019/src/optional_catch_binding.rs:20-26

The test code references OptionalCatchBinding which no longer exists:

use crate::optional_catch_binding::OptionalCatchBinding;

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        visit_mut_pass(OptionalCatchBinding),  // ❌ Does not exist
    )
}

Issue: The struct OptionalCatchBinding was removed but tests still reference it. This violates instruction #6 from CLAUDE.md: "When instructed to fix tests, do not remove or modify existing tests."

Fix: Update the test helper to use the new API:

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        optional_catch_binding(),
    )
}

⚠️ High Priority Issues

3. Inconsistent Identifier Naming

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:18

The new implementation uses "unused" while the original used "e":

• Old: private_ident!("e")
• New: private_ident!("unused")

Concern: This changes the output of the transformation, which may:

1. Break existing snapshots/golden tests
2. Affect debugging experience (stack traces will show different variable names)
3. Potentially cause subtle behavior changes if any code relies on the identifier name

Recommendation: Keep "e" for consistency unless there's a specific reason to change it.

4. TODO Comment Issue

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:16-17

// TODO: Do not use private_ident! here.
// All private identifiers should be tracked using TraverseCtx.

Concern: This TODO indicates the implementation is incomplete and using a suboptimal approach.

Question: Is this TODO addressing a known technical debt, or does it indicate this PR needs more work before merging?

📋 Code Quality & Best Practices

5. Performance Considerations ✅

Per CLAUDE.md #1: "Write performant code. Always prefer performance over other things."

Good: This refactor should improve performance by:

• Reducing the number of AST traversals
• Integrating into the unified transformer pipeline
• Eliminating overhead from the legacy visitor pattern

Suggestion: Consider adding performance benchmarks to validate the improvement claim in the PR title.

6. Documentation ⚠️

Per CLAUDE.md #7: "Write documentation for your code."

Missing:

• No doc comments on the hook() function
• No explanation of what OptionalCatchBindingPass does
• The module-level documentation is absent

Recommendation: Add basic documentation:

/// Transforms optional catch bindings (ES2019) into catch clauses with explicit parameters.
///
/// # Example
/// ```js
/// // Input
/// try {} catch {}
///
/// // Output  
/// try {} catch (e) {}
/// ```
pub fn hook() -> impl VisitMutHook<TraverseCtx> {
    OptionalCatchBindingPass {}
}

7. Unit Tests ⚠️

Per CLAUDE.md #5: "Write unit tests for your code."

Status: Tests exist but are broken (see issue #2). Once fixed, verify they still cover:

• Basic optional catch binding transformation
• Name collision scenarios (catch_binding_name_collision_1 and catch_binding_name_collision_2)

🔒 Security Considerations

No security concerns identified. The transformation is straightforward and doesn't introduce vulnerabilities.

📊 Test Coverage

Current Status: ❌ Tests fail to compile
After Fix: ✅ Should maintain existing coverage (3 test cases)

Recommendation: After fixing the compilation issues, run:

cargo test --package swc_ecma_compat_es2019 optional_catch_binding

✅ What's Good

1. Architecture: The migration to the hook-based system is the right direction
2. Simplicity: The implementation is clean and minimal
3. Integration: Good use of OptionalHook pattern for conditional transform application
4. Cargo.lock: Properly updated

🎯 Action Items

Must Fix Before Merge:

1. ✅ Fix type error by adding .into() (1 line change)
2. ✅ Fix broken tests by updating tr() helper
3. ⚠️ Decide on identifier name ("e" vs "unused")
4. ⚠️ Address or remove the TODO comment

Nice to Have:

5. Add documentation
6. Run performance benchmarks to validate the "perf" claim
7. Run cargo fmt --all before committing (per CLAUDE.md Unignore "431ecef8c85d4d24.js" and "8386fbff927a9e0e.js". #8)

Verdict

Status: ❌ Cannot Merge - Compilation errors present

This is a well-intentioned refactoring that moves in the right direction architecturally, but it has critical bugs that prevent it from compiling. The fixes are straightforward and should only take a few minutes.

Estimated effort to fix: ~10 minutes for required changes

PR Review: Merge optional_catch_binding Transform

Thank you for working on merging the optional_catch_binding transform into the new transformer architecture! This is a good refactoring effort. Here are my findings:

🐛 Critical Issues

1. Incorrect Cargo Version Dependency

Location: crates/swc_ecma_compat_es2019/Cargo.toml:21

The PR adds a dependency on swc_ecma_transformer = "0.1.0", but the actual version is 1.0.0:

swc_ecma_transformer = { version = "0.1.0", path = "../swc_ecma_transformer" }

Fix Required:

swc_ecma_transformer = { version = "1.0.0", path = "../swc_ecma_transformer" }

This is causing the build to fail with:

error: failed to select a version for the requirement `swc_ecma_transformer = "^0.1.0"`
candidate versions found which didn't match: 1.0.0

2. Broken Test References

Location: crates/swc_ecma_compat_es2019/src/optional_catch_binding.rs:20

The test code still references the old OptionalCatchBinding struct that was removed:

use crate::optional_catch_binding::OptionalCatchBinding;

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        visit_mut_pass(OptionalCatchBinding),  // ❌ This struct no longer exists
    )
}

Fix Required: Update the tests to use the new public API:

use super::optional_catch_binding;

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        optional_catch_binding(),
    )
}

⚠️ Semantic Changes

3. Generated Identifier Name Changed

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:18

The generated catch binding name was changed from "e" to "unused":

// Old implementation (removed):
node.param = Some(private_ident!("e").into());

// New implementation:
node.param = Some(Pat::Ident(private_ident!("unused")));

Impact:

• This changes the output of the transform, which could affect:
  • Snapshot tests that check generated code
  • Users who depend on consistent output for debugging
  • Minified output size (slightly larger: "unused" vs "e")

Recommendation:

• Either revert to "e" to maintain consistency with existing behavior
• Or document this as a breaking change if intentional
• Consider following the TODO comment and using TraverseCtx to generate a truly unique identifier

📋 Code Quality Observations

4. Performance Consideration

The new implementation follows the project's performance-first principle by using the hook-based architecture, which should provide better performance by avoiding unnecessary traversals. ✅

5. Missing Imports Cleanup

Location: crates/swc_ecma_compat_es2019/src/optional_catch_binding.rs:2-4

After the refactoring, these imports are no longer used but remain in the file:

use swc_ecma_utils::private_ident;  // ❌ Not used anymore
use swc_ecma_visit::{noop_visit_mut_type, visit_mut_pass, VisitMut, VisitMutWith};  // ❌ Not used
use swc_trace_macro::swc_trace;  // ❌ Not used

Recommendation: Remove unused imports for cleaner code.

6. TODO Comment

The implementation includes a TODO about using TraverseCtx for identifier generation. This is good practice for tracking technical debt. ✅

✅ Positive Aspects

1. Architecture Improvement: Successfully migrates to the new hook-based transformer architecture
2. Code Consolidation: Reduces duplication by centralizing the transform logic
3. Follows Project Conventions: Properly uses OptionalHook pattern for conditional transforms
4. Maintains Test Coverage: Tests are preserved (though they need fixing)

🔧 Action Items

Must Fix Before Merge:

1. Update Cargo.toml version from "0.1.0" to "1.0.0"
2. Fix broken test code to use the new API
3. Decide on identifier name ("e" vs "unused") and apply consistently
4. Remove unused imports

Nice to Have:
5. Run cargo fmt --all as per CLAUDE.md guidelines
6. Consider implementing the TODO about using TraverseCtx for identifier generation

Overall Assessment: Good refactoring direction, but needs fixes before merging. The main issues are straightforward to resolve.

Code Review for PR #11313: perf(es/compat): Merge optional_catch_binding

Overview

This PR merges the optional_catch_binding transformation into the new hook-based transformer architecture, which is a performance optimization to reduce the number of AST traversals.

Issues Found

🔴 Critical: Version Mismatch in Cargo.toml

Location: crates/swc_ecma_compat_es2019/Cargo.toml:21

The dependency specifies:

swc_ecma_transformer = { version = "0.1.0", path = "../swc_ecma_transformer" }

But the actual package version in crates/swc_ecma_transformer/Cargo.toml:10 is:

version = "1.0.0"

Impact: This will cause build failures. The version should be "1.0.0" instead of "0.1.0".

🟡 Inconsistent Variable Naming

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:18

The new implementation uses:

node.param = Some(Pat::Ident(private_ident!("unused")));

But the test snapshots show the expected output uses e or e1 (e.g., catch_binding_name_collision_1.js shows catch (e1)).

The old implementation used "e":

cc.param = Some(private_ident!("e").into());

Impact: This changes the output variable name from e to unused, which will break existing tests and may affect downstream tools that expect consistent naming.

Recommendation: Use "e" to match the previous behavior and test expectations.

🟡 Missing Test Update

Location: crates/swc_ecma_compat_es2019/src/optional_catch_binding.rs:20-26

The test code still references the old OptionalCatchBinding struct:

use crate::optional_catch_binding::OptionalCatchBinding;

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        visit_mut_pass(OptionalCatchBinding),
    )
}

But this struct no longer exists in the file after the refactor.

Impact: Tests will fail to compile.

Recommendation: The tests should be updated to use the new optional_catch_binding() function, or removed if they're now redundant with the transformer's tests.

Positive Aspects

✅ Good architectural improvement - Moving to the hook-based system reduces AST traversals
✅ Proper integration - Correctly integrated into preset_env using the options pattern
✅ Clean code structure - The new implementation is simpler and follows the established pattern
✅ Documentation - Good TODO comment about tracking private identifiers via TraverseCtx

Additional Observations

1. Performance: This refactor should improve performance by eliminating a separate AST traversal ✅

2. Code Quality: The hook-based approach is cleaner than the old VisitMut pattern ✅

3. Consistency: Follows the same pattern as other ES20XX transforms in the codebase ✅

Recommendations

1. Fix the version mismatch in Cargo.toml (change 0.1.0 → 1.0.0)
2. Change variable name from "unused" to "e" for consistency with existing behavior
3. Update or remove the old tests in the optional_catch_binding.rs file
4. Run tests to ensure all snapshots still pass after the changes

Overall, this is a solid refactor with good performance benefits, but needs these fixes before merging.

🤖 Generated with Claude Code

Pull Request Review: perf(es/compat): Merge optional_catch_binding

Overview

This PR refactors the optional_catch_binding transform by migrating it from the swc_ecma_compat_es2019 crate to the new hook-based architecture in swc_ecma_transformer. This is part of a larger effort to consolidate transforms and improve performance.

✅ Positive Aspects

1. Architecture Improvement: Moving to the hook-based system aligns with the modern architecture and enables better performance through unified traversal.

2. Consistent Pattern: The implementation follows the established pattern seen in other ES2019+ transforms.

3. Backward Compatibility: The old API is maintained through a wrapper that creates the new options, ensuring existing code continues to work.

🔴 Critical Issues

1. Broken Tests (crates/swc_ecma_compat_es2019/src/optional_catch_binding.rs:20-26)

The test module references OptionalCatchBinding struct that was removed:

use crate::optional_catch_binding::OptionalCatchBinding;

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        visit_mut_pass(OptionalCatchBinding),
    )
}

Issue: OptionalCatchBinding struct no longer exists in this crate - it was moved to swc_ecma_transformer.

Fix Required: Tests should be updated to use the new optional_catch_binding() function:

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        super::optional_catch_binding(),
    )
}

2. Identifier Name Inconsistency (crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:18)

node.param = Some(Pat::Ident(private_ident!("unused")));

Issue: The old implementation used "e" as the identifier name, but the new one uses "unused". This changes the output and could break snapshot tests.

Recommendation: Either:

• Use the same identifier name ("e") for consistency, OR
• Update all snapshot tests to reflect the new identifier name

The TODO comment on line 16-17 is good, but this inconsistency should be addressed now.

⚠️ Issues to Address

3. Missing Traversal Context (crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:16-17)

The TODO comment correctly identifies that private_ident! should not be used:

// TODO: Do not use private_ident! here.
// All private identifiers should be tracked using TraverseCtx.

Issue: Using private_ident! doesn't integrate with the traversal context's identifier tracking system, which could lead to identifier collisions in edge cases.

Recommendation: This should be addressed before merging, not left as a TODO. The TraverseCtx parameter should be used to generate unique identifiers.

4. Removed Visitor Child Traversal (crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:14)

The old implementation had:

fn visit_mut_catch_clause(&mut self, cc: &mut CatchClause) {
    cc.visit_mut_children_with(self);  // ← This line
    // ...
}

The new hook-based implementation doesn't explicitly traverse children.

Question: Does the hook system automatically handle child traversal? If not, this could miss nested catch clauses. Please verify this is handled correctly by the framework.

💡 Performance Considerations

Positive: This refactoring aligns with CLAUDE.md guideline #1 (Write performant code) by enabling:

• Unified AST traversal across multiple transforms
• Reduced overhead from multiple separate visitor passes
• Better cache locality

Verification Needed: Benchmark results from CI will validate the performance improvements.

🧪 Test Coverage

Issue: The PR modifies test infrastructure but the tests themselves are broken (see Issue #1).

Required Actions:

1. Fix the compilation error in the test module
2. Ensure all existing tests pass with the new implementation
3. Verify snapshot tests are updated if identifier name changed
4. Consider adding a test that verifies the new hook integrates correctly with swc_ecma_transformer

📋 Additional Observations

1. Dependency Addition: Adding swc_ecma_transformer as a dependency of swc_ecma_compat_es2019 creates a potential circular dependency concern. Verify this doesn't cause issues in the dependency graph.

2. CI Failures: Multiple checks are failing:

   • Cargo clippy - Likely due to the broken test code
   • Cargo shear - May be related to the new dependency
   • Check license of dependencies - Should be investigated

3. Documentation: Consider adding a comment explaining why the wrapper function now creates a full transformer options struct just to enable one feature.

📝 Recommendations Summary

Before merging:

1. ✅ Fix broken tests that reference removed OptionalCatchBinding struct
2. ✅ Resolve identifier name inconsistency ("e" vs "unused")
3. ✅ Implement proper identifier tracking using TraverseCtx (remove TODO)
4. ✅ Verify child traversal is handled correctly
5. ✅ Ensure all CI checks pass
6. ✅ Update any affected snapshot tests

Overall: This is a good refactoring that moves in the right direction architecturally. However, the broken tests and identifier inconsistencies need to be resolved before merging. The performance benefits are promising, but correctness must be ensured first.

Review performed in accordance with CLAUDE.md guidelines focusing on performance, code quality, and test coverage.

Code Review for PR #11313

Thank you for working on merging the optional_catch_binding transform! This is a good performance optimization to consolidate transforms into the new hook-based architecture. However, I've identified several issues that need to be addressed before this can be merged.

Critical Issues

1. Incorrect Dependency Version ❌

Location: crates/swc_ecma_compat_es2019/Cargo.toml:21

swc_ecma_transformer = { version = "0.1.0", path = "../swc_ecma_transformer" }

Issue: The version is specified as 0.1.0, but swc_ecma_transformer/Cargo.toml shows the actual version is 1.0.0. This causes a cargo dependency resolution error:

error: failed to select a version for the requirement `swc_ecma_transformer = "^0.1.0"`
candidate versions found which didn't match: 1.0.0

Fix: Change the version to "1.0.0":

swc_ecma_transformer = { version = "1.0.0", path = "../swc_ecma_transformer" }

2. Broken Tests ❌

Location: crates/swc_ecma_compat_es2019/src/optional_catch_binding.rs:20

The tests still reference the deleted OptionalCatchBinding struct:

use crate::optional_catch_binding::OptionalCatchBinding;

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        visit_mut_pass(OptionalCatchBinding),  // This no longer exists!
    )
}

Issue: According to CLAUDE.md guideline #6: "When instructed to fix tests, do not remove or modify existing tests." The tests are now broken because they reference a deleted type.

Fix: The tests should be updated to use the new API:

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        super::optional_catch_binding(),
    )
}

Major Concerns

3. Changed Identifier Name (Potential Breaking Change) ⚠️

Locations:

• Old: crates/swc_ecma_compat_es2019/src/optional_catch_binding.rs (deleted code)
• New: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:18

Change:

- cc.param = Some(private_ident!("e").into());
+ node.param = Some(Pat::Ident(private_ident!("unused")));

Issue: The generated identifier name changed from "e" to "unused". While both are private identifiers, this could be a breaking change if:

1. Users have tests that check the generated code output
2. Source maps or debugging tools rely on consistent naming
3. The PR description doesn't mention this as an intentional change

Questions:

• Is this identifier name change intentional?
• Should it be documented as a breaking change?
• The TODO comment suggests this implementation is temporary—should we keep "e" for consistency until the proper implementation?

4. Incomplete Implementation (TODO) ⚠️

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:16-17

// TODO: Do not use private_ident! here.
// All private identifiers should be tracked using TraverseCtx.

Issue: This TODO suggests the current implementation doesn't follow the intended architecture pattern. This could lead to:

1. Identifier conflicts if not properly tracked
2. Future technical debt
3. Inconsistency with other transforms in the codebase

Question: Should this be completed before merging, or is it acceptable to merge with this TODO and address it in a follow-up PR?

Code Quality & Best Practices

Positive Aspects ✅

1. Performance-focused: Merging transforms into the hook-based system aligns with CLAUDE.md guideline ecmascript parser #1 (performance first)
2. Clean architecture: Using OptionalHook pattern for conditional transforms is elegant
3. Proper integration: The preset_env integration correctly sets the flag instead of using the old pass system
4. Hook utilities: Added necessary enter_catch_clause/exit_catch_clause methods to hook_utils.rs

Documentation 📝

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs

According to CLAUDE.md guideline #7: "Write documentation for your code."

Consider adding module-level documentation:

//! Hook for transforming ES2019 optional catch binding syntax.
//!
//! Transforms `try {} catch {}` into `try {} catch (unused) {}`
//! to support environments that don't support optional catch bindings.

Security & Performance

• Security: ✅ No security concerns identified
• Performance: ✅ Hook-based approach should be more efficient than separate visitor passes

Test Coverage

Missing: No new tests were added for the hook-based implementation. While the existing tests in swc_ecma_compat_es2019 are maintained, consider:

1. Adding tests directly in swc_ecma_transformer for the hook
2. Verifying the tests still pass after fixing the broken references

Recommendations

Must Fix Before Merge:

1. ✅ Update dependency version from 0.1.0 to 1.0.0
2. ✅ Fix broken test references to OptionalCatchBinding

Should Consider:

3. ⚠️ Clarify whether the identifier name change ("e" → "unused") is intentional
4. ⚠️ Address or document the TODO about using TraverseCtx
5. 📝 Add module-level documentation

Before Committing:

6. Run cargo fmt --all (per CLAUDE.md Unignore "431ecef8c85d4d24.js" and "8386fbff927a9e0e.js". #8)
7. Verify all tests pass

Summary

This is a valuable performance optimization that consolidates the optional catch binding transform into the new architecture. However, it currently has two critical bugs (wrong dependency version and broken tests) that prevent it from building successfully. Once these are fixed, the PR should be in good shape, though the identifier name change and TODO comment should be clarified.

Great work on the refactoring overall! The hook-based approach is much cleaner than the old visitor pattern.

Pull Request Review - PR #11313

Summary

This PR refactors the optional_catch_binding transform by merging it into the new swc_ecma_transformer architecture. The goal is performance optimization by consolidating transforms into a single-pass compiler system.

Issues Found

🔴 Critical: Incorrect Dependency Version

Location: crates/swc_ecma_compat_es2019/Cargo.toml:21

swc_ecma_transformer = { version = "0.1.0", path = "../swc_ecma_transformer" }

Problem: The dependency specifies version 0.1.0, but swc_ecma_transformer/Cargo.toml:10 shows the actual version is 1.0.0. This causes a build failure:

error: failed to select a version for the requirement `swc_ecma_transformer = "^0.1.0"`
candidate versions found which didn't match: 1.0.0

Fix: Update to the correct version:

swc_ecma_transformer = { version = "1.0.0", path = "../swc_ecma_transformer" }

⚠️ Warning: Broken Test References

Location: crates/swc_ecma_compat_es2019/src/optional_catch_binding.rs:20

use crate::optional_catch_binding::OptionalCatchBinding;

Problem: The tests reference OptionalCatchBinding struct which was removed in line 6 of the same file. The old visitor implementation was deleted but the test code still tries to import and use it.

Impact: Tests will fail to compile. The tests should either:

1. Use the new optional_catch_binding() function directly, or
2. Be removed if they're covered by tests in swc_ecma_transformer

🔧 Code Quality Issue: Inconsistent Identifier Name

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:18

Old code (from swc_ecma_compat_es2019):

cc.param = Some(private_ident!("e").into());

New code:

node.param = Some(Pat::Ident(private_ident!("unused")));

Issue: The generated identifier changed from "e" to "unused". While the PR author left a TODO comment about using TraverseCtx (which is good), this behavior change could potentially affect output consistency or tests that expect the identifier name "e".

Recommendation: Keep the identifier name consistent ("e") unless there's a specific reason for the change. If the change is intentional, it should be noted in the PR description as a breaking change.

📝 Minor: Missing Documentation

Per CLAUDE.md rule #7: "Write documentation for your code."

The new es2019/optional_catch_binding.rs module lacks module-level documentation explaining:

• What this transform does
• Why optional catch bindings need transformation
• ES2019 feature compatibility context

Suggestion:

//! Transform for ES2019 optional catch binding.
//!
//! This transform converts catch clauses without parameters to catch clauses with
//! an unused parameter binding, for compatibility with engines that don't support
//! optional catch bindings.
//!
//! # Example
//!
//! Input:
//! ```javascript
//! try { } catch { }
//! ```
//!
//! Output:
//! ```javascript
//! try { } catch (e) { }
//! ```

Positive Observations

✅ Good architectural refactoring - Moving to a hook-based system for single-pass compilation aligns with performance goals (CLAUDE.md rule #1)

✅ Clean hook implementation - The OptionalHook pattern is elegant and allows optional transforms

✅ TODO comment present - Line 16 correctly notes that private_ident! should be replaced with TraverseCtx tracking

✅ Proper integration - The preset_env integration looks correct (lines 188-190 in lib.rs)

Test Coverage Concerns

The PR modifies the public API of swc_ecma_compat_es2019 but the test section still references old code. Please verify:

1. Are the existing tests in optional_catch_binding.rs still needed?
2. Should tests be added to swc_ecma_transformer instead?
3. Do integration tests cover the name collision scenarios (catch_binding_name_collision_1, catch_binding_name_collision_2)?

Action Items

Must Fix Before Merge:

• Fix Cargo.toml version from 0.1.0 to 1.0.0
• Fix or remove broken test code

Should Address:

• Clarify identifier name change ("e" → "unused")
• Add module-level documentation
• Verify test coverage is maintained

Nice to Have:

• Run cargo fmt --all as per CLAUDE.md rule Unignore "431ecef8c85d4d24.js" and "8386fbff927a9e0e.js". #8 (though CI should handle this)

Overall, this is a solid refactoring for performance optimization, but needs the critical dependency version fix before it can build successfully.

Code Review for PR #11313: perf(es/compat): Merge optional_catch_binding

Thank you for working on consolidating the optional_catch_binding transform into the new swc_ecma_transformer architecture! This is a good refactoring effort. Here's my feedback:

✅ Positive Aspects

1. Architectural Improvement: Moving the transform to the hook-based system in swc_ecma_transformer aligns with the modernization effort.
2. Code Simplification: The new implementation is more concise and integrates better with the transformer pipeline.
3. Consistent Pattern: Using OptionalHook follows the established pattern for conditional transforms.

🔴 Critical Issues

1. Version Mismatch in Cargo.toml ⚠️

Location: crates/swc_ecma_compat_es2019/Cargo.toml:21

swc_ecma_transformer = { version = "0.1.0", path = "../swc_ecma_transformer" }

Issue: The actual version in swc_ecma_transformer/Cargo.toml is 1.0.0, not 0.1.0. This causes the build to fail:

error: failed to select a version for the requirement `swc_ecma_transformer = "^0.1.0"`
candidate versions found which didn't match: 1.0.0

Fix: Update to:

swc_ecma_transformer = { version = "1.0.0", path = "../swc_ecma_transformer" }

2. Broken Tests ⚠️

Location: crates/swc_ecma_compat_es2019/src/optional_catch_binding.rs:20-26

The test code references OptionalCatchBinding which no longer exists after the refactor:

use crate::optional_catch_binding::OptionalCatchBinding;  // ❌ This no longer exists

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        visit_mut_pass(OptionalCatchBinding),  // ❌ Will not compile
    )
}

Fix: Update the test to use the new optional_catch_binding() function:

pub fn tr() -> impl Pass {
    (
        resolver(Mark::new(), Mark::new(), false),
        super::optional_catch_binding(),
    )
}

And remove the import of the deleted struct.

🟡 Medium Priority Issues

3. Inconsistent Parameter Naming

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:18

The generated identifier name changed from "e" to "unused":

Before: private_ident!("e")
After: private_ident!("unused")

Concern: This changes the generated code output, which could:

• Break existing snapshot tests
• Affect users who depend on specific generated code patterns
• Change behavior in edge cases where unused might collide with existing variables

Recommendation: Either:

1. Keep using "e" for consistency with the old behavior, OR
2. Document this as a BREAKING CHANGE in the PR description and update snapshot tests accordingly

4. TODO Comment Needs Action

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:16-17

// TODO: Do not use private_ident! here.
// All private identifiers should be tracked using TraverseCtx.

Issue: The TODO suggests using TraverseCtx instead of private_ident!, but this isn't implemented yet.

Question: Should this be addressed in this PR, or is it acceptable to defer to a future PR? If deferred, consider creating a follow-up issue.

📝 Minor Issues / Suggestions

5. Missing Context Documentation

Location: crates/swc_ecma_transformer/src/es2019/optional_catch_binding.rs:14

The unused _: &mut TraverseCtx parameter should have a comment explaining why it's not used yet (relates to the TODO above).

fn enter_catch_clause(&mut self, node: &mut CatchClause, _ctx: &mut TraverseCtx) {
    // ctx will be used for identifier tracking in the future (see TODO above)

6. Consider Adding Documentation

Per CLAUDE.md guideline #7, consider adding doc comments to the public hook() function:

/// Creates a hook that adds synthetic parameter bindings to catch clauses
/// without explicit parameter declarations (ES2019 optional catch binding feature).
pub fn hook() -> impl VisitMutHook<TraverseCtx> {
    OptionalCatchBindingPass {}
}

✅ Things Done Well

1. Follows CLAUDE.md Guidelines:

   • ✅ Uses &str for Atom (implicit via private_ident!)
   • ✅ Code is in English
   • ✅ No unstable features used
   • ✅ Existing tests preserved (structure-wise)

2. Performance: The new hook-based approach should be more efficient as it integrates better with the single-pass transformer architecture.

3. Integration: The preset_env integration looks correct, using the feature flag pattern.

🎯 Action Items Before Merge

• CRITICAL: Fix version mismatch in Cargo.toml (change 0.1.0 → 1.0.0)
• CRITICAL: Fix broken test code in optional_catch_binding.rs:20-26
• HIGH: Run cargo test to verify all tests pass
• HIGH: Decide on identifier naming (e vs unused) and update snapshots if needed
• MEDIUM: Document breaking changes in PR description if identifier naming changed
• LOW: Run cargo fmt --all (per CLAUDE.md Unignore "431ecef8c85d4d24.js" and "8386fbff927a9e0e.js". #8)
• LOW: Consider addressing the TODO or creating a follow-up issue

🔍 Testing Recommendations

Please ensure these tests pass:

cargo test -p swc_ecma_compat_es2019 optional_catch_binding
cargo test -p swc_ecma_transformer
cargo test -p swc_ecma_preset_env

Overall, this is a solid refactoring with clear architectural benefits. The main blockers are the version mismatch and broken test references, which should be straightforward to fix. 👍